Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Updates and Cleanup #89

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hashworks
Copy link

I just released v1.6.1 to the Arch Linux extra repo. But it would be great if you could release a v1.7.0 with the latest go & dep versions.

Closes #88.

@hashworks
Copy link
Author

hashworks commented Jul 3, 2024

Supersedes #85

@hashworks
Copy link
Author

Tested with latest 7.5.0, works fine.

@hashworks
Copy link
Author

@jonnenauha would you like me to adjust anything here?

@steinbrueckri
Copy link

bump 🎉

@@ -1,5 +1,16 @@
module github.com/jonnenauha/prometheus_varnish_exporter

go 1.12
go 1.22
Copy link
Owner

@jonnenauha jonnenauha Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would go 1.22 be required? I get the version bump if deps require it, but im not using packages that need new APIs from later go releases.

Prom seems to require go 1.21 so we should target that. https://github.com/prometheus/client_golang/blob/main/go.mod#L3

Despite what the go.mod says here, whatever go version you have will be used to build the binary. Its not like if you have go 1.23 installed and build it, it's not going to be be like you built it with go 1.12. go mod tells you the minimum version you can use to build the code. So if you are using features released in later go versions you can enforce it here.

But I agree bumping the version up to 1.21 is sensible. It can probably enable some compiler features versus 1.12.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more note, the travis ci go version update is ofc importatnt as it pushes the offiicial releases to github. So that is great, I was more talking about the go.mod changes here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just me bumping everything to latest, you are correct, we can set this to 1.21.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: Actually, since 1.23 has been released this is no longer correct. I wouldn't set 1.21 here since it is no longer supported:

Each major Go release is supported until there are two newer major releases.

@@ -114,25 +114,25 @@ type group struct {

var (
groups = []group{
group{name: "backend", prefixes: []string{
{name: "backend", prefixes: []string{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what auto formatter are you using to remove the struct name here? i have never seen the official go fmt do this? I think this is an unnecessary change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the result of gofmt -s, it "simplifies" code. Defining the struct type here is redundant.

@@ -229,7 +229,7 @@ func cleanBackendName(name string) string {
if strings.HasPrefix(name, "reload_") {
dot := strings.Index(name, ".")
if dot != -1 {
name = name[dot + 1:]
name = name[dot+1:]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this certainly is not go fmt either right?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's just standard go fmt.

$ git status
On branch master
Your branch is up to date with 'origin/master'.
$ go fmt ./prometheus.go
$ git diff
  diff --git a/prometheus.go b/prometheus.go
  index c54f642..533051e 100644
  --- a/prometheus.go
  +++ b/prometheus.go
  @@ -229,7 +229,7 @@ func cleanBackendName(name string) string {
      if strings.HasPrefix(name, "reload_") {
          dot := strings.Index(name, ".")
          if dot != -1 {
  -           name = name[dot + 1:]
  +           name = name[dot+1:]
          }
      }

@@ -1,8 +1,8 @@
#!/bin/bash
#!/bin/env bash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so did you actually change any logic here or just run it through some auto formatter? i dont see the point, afaik the script was working fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither. Yes, /bin/bash will work fine on most systems, but in rare cases this path is not correct (NixOS comes to mind). /bin/env bash ensures it works across the board. It's just good practice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Prometheus Go library
3 participants